aspect: fix flake8 E402/E305/E501 in aspect.py#2740
Open
brendancol wants to merge 5 commits into
Open
Conversation
Move the xrspatial.geodesic import up with the other top-of-file imports (it sat below _geodesic_cuda_dims, triggering E402 + E305). Wrap two over-long _run_gpu_geodesic_aspect kernel-launch calls (E501). Style only, no behavioural change. isort reordering intentionally not applied: slope.py/curvature.py use one-import-per-line for xrspatial.utils, and raw isort would make aspect inconsistent with them.
brendancol
commented
May 30, 2026
Contributor
Author
brendancol
left a comment
brendancol
left a comment
There was a problem hiding this comment.
PR Review: aspect: fix flake8 E402/E305/E501 in aspect.py
Blockers
None.
Suggestions
None.
Nits
None.
What looks good
- The diff is exactly the three intended changes and nothing else. No adjacent code touched.
- Relocating the
xrspatial.geodesicimport to the top is semantically identical. The module imports cleanly with no dependency on_geodesic_cuda_dims, confirmed by the passing test run. This also resolves the real readability problem of an import hidden below a function def. - Both line-wraps are whitespace-only inside the argument lists of the
_run_gpu_geodesic_aspect[...]kernel launches. No behavioural change. - The decision to skip the isort reordering is correct: slope.py and curvature.py use one-import-per-line for
xrspatial.utils, so applying the raw isort diff would make aspect the odd one out. Documented in the PR body rather than silently suppressed.
Checklist
- Algorithm matches reference/paper (no algorithm change)
- All implemented backends produce consistent results (static reformat; shared source)
- NaN handling is correct (untouched)
- Edge cases are covered by tests (existing suite exercises every changed line)
- Dask chunk boundaries handled correctly (untouched)
- No premature materialization or unnecessary copies (no compute change)
- Benchmark exists or is not needed (not needed for a style fix)
- README feature matrix updated (not applicable)
- Docstrings present and accurate (untouched)
Verified locally: flake8 xrspatial/aspect.py clean; pytest test_aspect.py 64 passed; pytest test_geodesic_aspect.py 18 passed.
brendancol
commented
May 31, 2026
Contributor
Author
brendancol
left a comment
brendancol
left a comment
There was a problem hiding this comment.
PR Review: aspect: fix flake8 E402/E305/E501 in aspect.py
(Re-review after merging origin/main to resolve the conflict with #2741.)
Blockers
None.
Suggestions
- Apply isort now. The original PR skipped isort because slope.py and curvature.py both used one-import-per-line for
xrspatial.utils, andisort --check-onlyflagged them too, so applying it would have made aspect the odd one out. That reasoning is now stale. PR #2690 (merged to main) reformatted slope.py to the grouped parenthesised style, and slope.py now passes isort. slope.py is aspect's direct structural twin: same_geodesic_cuda_dimshelper, same geodesic + utils import pattern. Running isort on aspect.py (xrspatial/aspect.py:14-32) produces the exact import layout slope.py now has, so applying it makes aspect consistent with the freshly-cleaned slope.py instead of inconsistent with it.
Nits
None.
What looks good
- Net diff vs main is just the three style fixes. The conflicting #2741
dtype=float32dask meta changes are in main now and merged cleanly; the merge preserved them (xrspatial/aspect.py:172, 182). - The geodesic import relocation (xrspatial/aspect.py:27) is import-order-safe, confirmed by 87 passing aspect+geodesic tests.
- Both kernel-launch wraps (xrspatial/aspect.py:219, 264) are whitespace-only inside argument lists. No behavioural change.
Checklist
- Algorithm matches reference (no algorithm change)
- All implemented backends consistent (static reformat, shared source)
- NaN handling correct (untouched)
- Edge cases covered by tests (existing suite exercises every changed line; 87 pass)
- Dask chunk boundaries correct (untouched; #2741 float32 meta preserved through merge)
- No premature materialization (no compute change)
- Benchmark exists or not needed (style fix, not needed)
- README feature matrix (not applicable)
- Docstrings present and accurate (untouched)
Address review suggestion on PR #2740. The original change skipped isort to stay consistent with slope.py/curvature.py one-import-per-line style. PR #2690 has since reformatted slope.py to the grouped parenthesised isort style, so aspect now matches its structural twin slope.py. isort and flake8 both clean. No behavioural change.
brendancol
commented
May 31, 2026
Contributor
Author
brendancol
left a comment
brendancol
left a comment
There was a problem hiding this comment.
PR Review (follow-up): isort suggestion addressed
The one suggestion from the previous review is fixed in 4475813.
- Ran isort on aspect.py. The import block now uses the grouped parenthesised layout matching slope.py (xrspatial/aspect.py:18-22). This also subsumes the manual E402 fix, since the geodesic import now sits at the top in sorted position.
flake8 xrspatial/aspect.pyclean;isort --check-only xrspatial/aspect.pyclean.- 87 aspect + geodesic tests pass.
No remaining Blockers, Suggestions, or Nits. Net diff vs main is the grouped-import reformat plus the two kernel-launch line wraps. No behavioural change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2683
What
from xrspatial.geodesic import (...)block from below_geodesic_cuda_dimsup to the other top-of-file imports. Fixes E402 (import not at top) and E305 (blank lines after function def)._run_gpu_geodesic_aspect[griddim, blockdim](...)kernel-launch calls. Fixes both E501 (101 and 109 chars).flake8 xrspatial/aspect.pynow reports zero violations.Categories addressed
Cat 1 (flake8 E-codes: E402, E305, E501).
Cat 4 (isort) was reviewed and intentionally not applied: slope.py and curvature.py use one-import-per-line for
xrspatial.utils, andisort --check-onlyflags them the same way. Applying the raw isort diff would collapse aspect's imports and make it inconsistent with its sibling terrain modules. No config widening, no# noqa.Cat 2, Cat 3, and Cat 5 (bare except / mutable defaults /
== None/ shadowed builtins) were grepped and are clean.No behavioural change
Static reformatting only. All four backends (numpy / cupy / dask+numpy / dask+cupy) share the same source, so no backend-specific verification is needed.
Test plan
flake8 xrspatial/aspect.pycleanpytest xrspatial/tests/test_aspect.py(64 passed)pytest xrspatial/tests/test_geodesic_aspect.py(18 passed)aspect/northness/eastness